Skip to content

Fix alias emitted in mapping #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 31, 2025
Merged

Fix alias emitted in mapping #125

merged 3 commits into from
Oct 31, 2025

Conversation

@ccoVeille
Copy link
Contributor

@ccoVeille ccoVeille commented Sep 19, 2025 •
edited
Loading

This partially rolls back the changes made with 926dd0b with #47

The colon was allowed in the characters allowed for anchor and alias
name. But it leads to ambiguity in the YAML structure.

This PR also makes sure to emit a space when an alias is used in a key of a mapping node.

Related to:

The issue appeared with the refactoring made with #47 that made sure to respect the YAML specification that allows having a : in an anchor and alias name.

The issue was reported by @mikefarah when he tried to migrate on go.yaml.in/yaml/v4

Copilot AI review requested due to automatic review settings September 19, 2025 22:11

This comment was marked as outdated.

Sign in to view

@ccoVeille ccoVeille marked this pull request as draft September 20, 2025 06:32
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Sep 20, 2025

I should have mentionned it. This is draft. As a proof of concept and for discussion purpose.

ccoVeille
ccoVeille commented Sep 20, 2025
View reviewed changes
@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from 851644f to 3d03825 Compare September 28, 2025 19:32
@ccoVeille ccoVeille marked this pull request as ready for review September 28, 2025 19:33
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Sep 28, 2025

cc @mikefarah @inteon

@ingydotnet ingydotnet force-pushed the fix-emit-alias-in-mapping branch from 3d03825 to d98cd1a Compare September 29, 2025 18:45
@ingydotnet
Copy link
Member

ingydotnet commented Sep 29, 2025

I would prefer that we revert allowing : in anchors.
The libyaml family has always been overly strict about chars allowed in anchors.
They only allow [-\w] chars.
We allowed more chars (all the 7bit ascii allowed chars) but we are currently far from spec compliant.
The spec allows all unicode chars minus the syntax chars it disallows.
I feel this is both unimportant and potentially dangerous.
Removing : will not bother anyone and will cause less problems (like the one yq reported).

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from d98cd1a to 7bb6299 Compare September 29, 2025 20:24
@ccoVeille ccoVeille requested a review from Copilot September 29, 2025 20:28
Copilot AI reviewed Sep 29, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch 2 times, most recently from ade13e8 to 8d9ab3d Compare October 2, 2025 02:36
@mikefarah
Copy link

mikefarah commented Oct 16, 2025

Where are we at with this PR? @ccoVeille / @ingydotnet ?

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from 8d9ab3d to ffe4314 Compare October 16, 2025 18:54
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Oct 16, 2025

@ingydotnet I rebased and fixed conflicts.

@ingydotnet ingydotnet force-pushed the fix-emit-alias-in-mapping branch from ffe4314 to edf4071 Compare October 23, 2025 21:43
stefanprodan
stefanprodan approved these changes Oct 24, 2025
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ingydotnet ingydotnet requested a review from SVilgelm October 31, 2025 21:52
SVilgelm
SVilgelm approved these changes Oct 31, 2025
View reviewed changes
Copy link
Contributor

@SVilgelm SVilgelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it only after reading all discussions

@ccoVeille @SVilgelm
Improve error reporting when comparing yaml.Node
4f369fb 
The DeepEqual asserter was too simple to cover this.
@ccoVeille @SVilgelm
Remove colon from characters allowed in anchor
6a9961c 
This partially rolls back the changes made with 926dd0b

The colon was allowed in the characters allowed for anchor and alias
name. But it leads to ambiguity in the YAML structure.
@ccoVeille @SVilgelm
Fix alias emitted in mapping
baafdbf 
The idea is to make sure that a YAML alias in a mapping is followed by
a space before the :

An issue appeared with the refactoring made recently that made sure
to respect the YAML specification that allows having a `:` in an anchor
and alias name.
@SVilgelm SVilgelm force-pushed the fix-emit-alias-in-mapping branch from edf4071 to baafdbf Compare October 31, 2025 22:41
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Oct 31, 2025 •
edited
Loading

Got it only after reading all discussions

Yes, it's a fun and complicated issue for a niche 😅

@SVilgelm SVilgelm merged commit 898e4c2 into yaml:main Oct 31, 2025
18 checks passed
@ccoVeille ccoVeille deleted the fix-emit-alias-in-mapping branch November 1, 2025 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ingydotnet ingydotnet ingydotnet approved these changes

@SVilgelm SVilgelm SVilgelm approved these changes

@stefanprodan stefanprodan stefanprodan approved these changes

@carloslima carloslima Awaiting requested review from carloslima

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ccoVeille @ingydotnet @mikefarah @SVilgelm @stefanprodan